Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make TimeZoneRegion::utc() return a TimeZoneRegion not an TimeZoneOffset #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tigitz
Copy link
Contributor

@tigitz tigitz commented Apr 27, 2022

I think it's reasonable from a DX perspective to expect TimeZoneRegion::utc() to return a TimeZoneRegion instead of the offset.

@tigitz
Copy link
Contributor Author

tigitz commented Jun 9, 2022

@BenMorel If you could take a quick look about this one, it's pretty straight forward. It's slightly related to #55

@jiripudil
Copy link
Contributor

Or perhaps there shouldn't be TimeZone::utc() at all, only leaving TimeZoneOffset::utc()? That's what Java does and it makes it super clear that it's an offset-based TZ.

I appreciate the shorthand form of TimeZone::utc() and I must admit we're using it a lot in our code, so both this PR and my proposal would be a breaking change, but if it would help clear some confusion, we could live with it; it's an easy change, after all.

@BenMorel
Copy link
Member

BenMorel commented Jun 12, 2022

Good point about TimeZoneRegion::utc() returning an offset, we definitely need to fix this in one way or another.

I did like the fact than you could just get UTC from the root TimeZone class, but it definitely makes it weird when called on TimeZoneRegion.

I don't think keeping an abstract static TimeZone::utc() makes sense though, it can probably be removed (in 0.5.0).

That leaves us with two methods to represent a UTC time-zone: TimeZoneOffset::utc() and TimeZoneRegion::utc(). Isn't this confusing? What should we typically use here?

Indeed, Java has a UTC constant on ZoneOffset, but not ZoneId. Maybe doing the same would introduce less confusion? I can imagine future issues popping from users asking what's the difference between TimeZoneOffset::utc() and TimeZoneRegion::utc().

@solodkiy
Copy link
Contributor

solodkiy commented Jun 12, 2022

Definitely don't want to see two different utc factory. Move utc() to TimeZoneOffset looks reasonable.
Do we have any real use case for TimeZoneRegion::utc()?

@BenMorel
Copy link
Member

@solodkiy I don't think so. You can still construct it manually with TimeZoneRegion::of('etc/UTC') anyway if required.

@BenMorel
Copy link
Member

Moving forward, I don't think it hurts much to keep TimeZone::utc() for now, that delegates to TimeZoneOffset::utc() as it currently does.

@tigitz Can you please just add the new method in TimeZoneRegion, but not replace calls to TimeZone::utc() with TimeZoneOffset::utc()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants